Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter for correct usage/usagePage when finding xkeys devices #93

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

Julusian
Copy link
Member

fixes #92

The filtering might be overly strict, and needs to be verified on windows and macos to ensure the required properties are defined.

Tested with a XK24 and XK12-Jog

@nytamin nytamin marked this pull request as ready for review October 25, 2023 07:43
@nytamin nytamin self-assigned this Oct 25, 2023
@nytamin
Copy link
Member

nytamin commented Oct 25, 2023

Hey @michaelhatPIengineering !
This PR basically adds the filter below (for browser WebHID only):

function isValidXkeysUsage(device: HIDDevice): boolean {
	return !!device.collections.find(
		(collection) => collection.type === 1 && collection.usage === 1 && collection.usagePage === 12
	)
}

Does this filter look reasonable to you? Do you think we should go ahead and merge this fix right away or would you like to test it with various devices first? (I could prepare a test script for you)

@michaelhatPIengineering
Copy link
Collaborator

@michaelhatPIengineering
Copy link
Collaborator

@KhangxLi
Copy link

it seems like the fix worked on Mac, but not on Windows

@KhangxLi
Copy link

looks like collection.type === 0 for XKeys XK-80 on Windows

@michaelhatPIengineering
Copy link
Collaborator

@Julusian
Copy link
Member Author

Julusian commented Nov 1, 2023

I have stripped out some of the conditions, and left it checking usagePage and vendorId.
It is validating the productId later, so I don't think it is necessary to do at this early stage, especially as the user prompt isnt doing a filter on productId.

I haven't added a writeLength check, as doing so requires digging deeply into multiple layers of arrays and objects and that makes me a little uneasy about the reliability of doing so. Although each of linux, macos and windows did produce what appeared to be the same data for it:

image

@Julusian Julusian force-pushed the fix/webhid-wrong-device branch from 7d09fe6 to 68f3e86 Compare November 1, 2023 15:46
@michaelhatPIengineering
Copy link
Collaborator

michaelhatPIengineering commented Nov 1, 2023 via email

@Julusian
Copy link
Member Author

Julusian commented Nov 1, 2023

ok, I have added that check. The function now looks like:

function isValidXkeysUsage(device: HIDDevice): boolean {
	if (device.vendorId !== XKEYS_VENDOR_ID) return false

	return !!device.collections.find((collection) => {
		if (collection.usagePage !== 12) return false

		// Check the write-length of the device is > 20
		return !!collection.outputReports?.find((report) => !!report.items?.find((item) => item.reportCount ?? 0 > 20))
	})
}

I have tested that this new version still works on windows, macos and linux

@michaelhatPIengineering
Copy link
Collaborator

michaelhatPIengineering commented Nov 1, 2023 via email

@Julusian Julusian requested a review from nytamin November 1, 2023 17:53
packages/webhid/src/methods.ts Outdated Show resolved Hide resolved
@nytamin nytamin merged commit f60ffdc into master Nov 2, 2023
8 checks passed
@nytamin nytamin deleted the fix/webhid-wrong-device branch November 2, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent "NotAllowedError: Failed to write the report." on Chrome v115+
4 participants